doc: document process.versions.modules #9901

kzurawel · 2016-12-01T16:22:03Z

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc process

Description of change

This commit adds a description of process.versions.modules,
based on the comment in src/node_version.h lines 47-50.

This commit adds a description of `process.versions.modules`, based on the comment in `src/node_version.h` lines 47-50.

Trott · 2016-12-01T21:35:41Z

doc/api/process.md

@@ -1651,7 +1651,9 @@ added: v0.2.0
 * {Object}

 The `process.versions` property returns an object listing the version strings of
-Node.js and its dependencies.
+Node.js and its dependencies. `process.versions.modules` indicates the current
+ABI version, which is increased whenever a C++ API changes. Node.js will refuse


Thanks for the contribution. I'm not sure if we have something in the docs that explains ABI, but if we do, making ABI a link to that explanation would be great.

I think this should be more of a "In addition, the modules property indicates ...", because this version contradicts the earlier sentence, the modules property is not a version of Node.js or its dependencies.

Perhaps it would be even better by enumerating each individual property, and describing it?

There's nothing in the docs that explains ABI currently. Could be a good addition to https://nodejs.org/dist/latest-v7.x/docs/api/addons.html at some point (/cc @nodejs/documentation). I wouldn't make it a condition for landing this PR tho

sam-github · 2016-12-13T18:07:30Z

@kzurawel are you still working on this?

kzurawel · 2016-12-13T18:23:43Z

I'm still "working" on this, but I'm not sure what needs to be done at this point.

sam-github · 2016-12-13T19:35:34Z

You need to read all comments, and if they suggest a change, implement it or explain why not. If they ask a question, answer it as a comment, or answer implicitly by pushing to the PR.

Specifically, in #9901 (comment)

I said:

I think this should be more of a "In addition, the modules property indicates ...", because this version contradicts the earlier sentence, the modules property is not a version of Node.js or its dependencies.

You should do this, or something similar, or comment on why you don't think it should be done.

Perhaps it would be even better by enumerating each individual property, and describing it?

This is phrased as a question, so you can answer it, or just do what I suggeest and change the doc format, or not change the doc format if you don't have the time or don't think its a good idea.

Trott · 2016-12-21T22:48:47Z

@kzurawel Are you still working on this?

(If not, does anyone else want to take it over and get it across the finish line?)

kzurawel · 2016-12-21T23:31:38Z

I think that's all, I'll leave describing the other properties for future work.

Trott · 2016-12-22T00:41:42Z

@sam-github Looks good to you?

addaleax

LGTM with a nit, thanks for this!

addaleax · 2016-12-22T07:19:55Z

doc/api/process.md

-Node.js and its dependencies.
+Node.js and its dependencies. In addition, `process.versions.modules` indicates
+the current ABI version, which is increased whenever a C++ API changes. Node.js
+will refuse to load native modules built for an older `modules` value.


nit: This sounds like Node will load native modules built for newer modules values, which isn’t the case… maybe something like for a different fits better?

node/src/node_version.h

Lines 46 to 52 in e03ee71

/**

* When this version number is changed, node.js will refuse

* to load older modules. This should be done whenever

* an API is broken in the C++ side, including in v8 or

* other dependencies.

*/

#define NODE_MODULE_VERSION 51 /* Node.js v7.0.0 */

Documentation text mirrors the only available documentation for this property.... an in-source comment. @addaleax are you saying the comment is wrong?

If so, this PR should change both the comment and the docs.

@sam-github No, I didn’t say the current text is wrong. It’s just incomplete and might therefore be misleading.

If so, this PR should change both the comment and the docs.

No objections to that, but, like, this isn’t worth holding up the PR or anything imho.

Right, so text should be something like "Node.js will refuse to load modules that weren't compiled against its own
module ABI number."

Trott · 2016-12-23T00:58:38Z

Landed in f368eee.

I'll update the text further (based on the back-and-forth between @addaleax and @sam-github) in a subsequent PR.

This commit adds a description of `process.versions.modules`, based on the comment in `src/node_version.h` lines 47-50. PR-URL: nodejs#9901 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

This commit adds a description of `process.versions.modules`, based on the comment in `src/node_version.h` lines 47-50. PR-URL: #9901 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

Refs: nodejs#9901 (comment)

PR-URL: nodejs#10419 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Refs: nodejs#9901 (comment)

This commit adds a description of `process.versions.modules`, based on the comment in `src/node_version.h` lines 47-50. PR-URL: #9901 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

PR-URL: nodejs#10419 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Refs: nodejs#9901 (comment)

PR-URL: #10419 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Refs: #9901 (comment)

doc: document process.versions.modules

80f97c2

This commit adds a description of `process.versions.modules`, based on the comment in `src/node_version.h` lines 47-50.

nodejs-github-bot added doc Issues and PRs related to the documentations. process Issues and PRs related to the process subsystem. labels Dec 1, 2016

Trott reviewed Dec 1, 2016

View reviewed changes

jasnell approved these changes Dec 5, 2016

View reviewed changes

Trott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Dec 6, 2016

doc: update wording for process.versions.modules

91d0465

nodejs-github-bot added the lts-watch-v6.x label Dec 21, 2016

jasnell approved these changes Dec 22, 2016

View reviewed changes

addaleax approved these changes Dec 22, 2016

View reviewed changes

Trott closed this Dec 23, 2016

Trott mentioned this pull request Dec 23, 2016

doc,src: clarify doc/comment about ABI number #10419

Closed

3 tasks

targos added land-on-v6.x and removed lts-watch-v6.x labels Dec 26, 2016

targos added lts-watch-v4.x and removed lts-watch-v4.x labels Dec 26, 2016

MylesBorins mentioned this pull request Dec 27, 2016

V6.9.3 proposal #10394

Merged

Trott added a commit to Trott/io.js that referenced this pull request Dec 28, 2016

doc: clarify information about ABI version

6755347

Refs: nodejs#9901 (comment)

evanlucas mentioned this pull request Jan 3, 2017

v7.4.0 release proposal #10589

Merged

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Jan 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: document process.versions.modules #9901

doc: document process.versions.modules #9901

kzurawel commented Dec 1, 2016

Trott Dec 1, 2016

sam-github Dec 5, 2016

jasnell Dec 7, 2016

sam-github commented Dec 13, 2016

kzurawel commented Dec 13, 2016

sam-github commented Dec 13, 2016

Trott commented Dec 21, 2016

kzurawel commented Dec 21, 2016

Trott commented Dec 22, 2016

addaleax left a comment

addaleax Dec 22, 2016

sam-github Dec 22, 2016

addaleax Dec 22, 2016

sam-github Dec 22, 2016

Trott commented Dec 23, 2016 •

edited by gibfahn

Loading

	/**
	* When this version number is changed, node.js will refuse
	* to load older modules. This should be done whenever
	* an API is broken in the C++ side, including in v8 or
	* other dependencies.
	*/
	#define NODE_MODULE_VERSION 51 /* Node.js v7.0.0 */

doc: document process.versions.modules #9901

doc: document process.versions.modules #9901

Conversation

kzurawel commented Dec 1, 2016

Checklist

Affected core subsystem(s)

Description of change

Trott Dec 1, 2016

Choose a reason for hiding this comment

sam-github Dec 5, 2016

Choose a reason for hiding this comment

jasnell Dec 7, 2016

Choose a reason for hiding this comment

sam-github commented Dec 13, 2016

kzurawel commented Dec 13, 2016

sam-github commented Dec 13, 2016

Trott commented Dec 21, 2016

kzurawel commented Dec 21, 2016

Trott commented Dec 22, 2016

addaleax left a comment

Choose a reason for hiding this comment

addaleax Dec 22, 2016

Choose a reason for hiding this comment

sam-github Dec 22, 2016

Choose a reason for hiding this comment

addaleax Dec 22, 2016

Choose a reason for hiding this comment

sam-github Dec 22, 2016

Choose a reason for hiding this comment

Trott commented Dec 23, 2016 • edited by gibfahn Loading

Trott commented Dec 23, 2016 •

edited by gibfahn

Loading